Skip to content

Fix hang on UTF-16 LE BOM file#636

Open
chrchr-github wants to merge 11 commits into
cppcheck-opensource:masterfrom
chrchr-github:chr_utf16
Open

Fix hang on UTF-16 LE BOM file#636
chrchr-github wants to merge 11 commits into
cppcheck-opensource:masterfrom
chrchr-github:chr_utf16

Conversation

@chrchr-github
Copy link
Copy Markdown
Contributor

@chrchr-github chrchr-github commented Mar 27, 2026 •
edited
Loading

fixes #637

@chrchr-github chrchr-github changed the title Fix hang on UTF- 16 LE BOM file Fix hang on UTF-16 LE BOM file Mar 27, 2026
@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Mar 27, 2026 •
edited
Loading

Yeah - there's several places which lack checks for EOF. I have WIP tests for that and other corner cases/gaps somewhere - something to be done in preparation of #566.

@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Apr 12, 2026

With #647 it should be possible to write a test which uses the input in question.

@chrchr-github
Copy link
Copy Markdown
Contributor Author

chrchr-github commented Apr 12, 2026

With #647 it should be possible to write a test which uses the input in question.

Can we just run the tests with all three options?

@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Apr 12, 2026

Can we just run the tests with all three options?

Yes, if somebody adds the functionality. It didn't seem straight forward to me so I haven't yet. Feel free to do so.

@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Apr 13, 2026

I think I took a wrong turn. That should trigger with regular unit test. So please add a test for it.

@firewave
Copy link
Copy Markdown
Collaborator

firewave commented Apr 13, 2026

I think I took a wrong turn. That should trigger with regular unit test. So please add a test for it.

Ah - you mentioned FileStream in the ticket. Still I think we should have a unit test (if missing).

@chrchr-github
Copy link
Copy Markdown
Contributor Author

chrchr-github commented May 13, 2026

So there is also some kind of platform divergence going on...

@chrchr-github
Copy link
Copy Markdown
Contributor Author

chrchr-github commented May 13, 2026

These (hanging) runs may be cancelled by the way:
https://github.com/cppcheck-opensource/simplecpp/actions/runs/25805772896
https://github.com/cppcheck-opensource/simplecpp/actions/runs/25805772942/job/75807948771

@chrchr-github
Copy link
Copy Markdown
Contributor Author

chrchr-github commented May 13, 2026 •
edited
Loading

So there is also some kind of platform divergence going on...

Of course f.write("\xFF\xFE\x3B\x00") does something different on Linux, because why wouldn't it?

~$ hexdump pytest.cpp 
0000000 bfc3 bec3 003b                         
0000006

@chrchr-github chrchr-github marked this pull request as ready for review May 13, 2026 18:21
@danmar
Copy link
Copy Markdown
Collaborator

danmar commented May 15, 2026

I guess CI will be fixed by #641

danmar
danmar reviewed May 15, 2026
View reviewed changes
Comment thread simplecpp.cpp
const int pk = peek();
auto ch = static_cast<unsigned char>(pk);
if (pk == EOF)
return ch;
Copy link
Copy Markdown
Collaborator

@danmar danmar May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate that we handle EOF. However, spontanously it does not seem optimal to return some abitrary ch value. It's implementation defined what value of EOF is right? If we would return 0xFF we at least know what the return value is. Or do we want to have the implementation defined return value?

Copy link
Copy Markdown
Collaborator

@firewave firewave May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to return EOF since we are a custom layer on top of getc(). And since we use the EOF macro it is not "arbitrary" but the one defined by the libc implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danmar danmar danmar left review comments

@firewave firewave firewave left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Hang on UTF-16 with BOM

3 participants

@chrchr-github @firewave @danmar